🤖 Kimi Code Review

Review Summary

This PR introduces important fixes for attestation validation and aggregation logic, along with some configuration updates. Here are the key findings:

Critical Issues

1. Attestation validation bug (fixed): The new validation checks in validate_attestation_data for head slot consistency are correct and necessary. The previous code was missing validation that the head checkpoint slot matches the actual block slot.

2. Fork choice aggregation bug (fixed): The merge of known and new aggregated payloads in update_safe_target is crucial. Without this, attestations that enter directly into the "known" pool (like proposer's own attestation) would be invisible to fork choice at interval 3.

Security Considerations

• Attestation validation completeness: The new validation rules (head slot must be ≥ target slot, checkpoint slots must match actual block slots) prevent malformed attestations that could confuse fork choice.
• No apparent security vulnerabilities in the changes.

Performance & Correctness

1. Memory efficiency: The new aggregate collection in aggregate_committee_signatures returns Vec<SignedAggregatedAttestation> instead of immediately publishing, which is cleaner but creates an intermediate vector. This is acceptable for the expected committee sizes.

2. Network configuration change: Changing from devnet3 to devnet0 in p2p/src/lib.rs appears intentional for testnet coordination.

Code Quality

• Error handling: Good use of inspect_err for logging P2P send failures
• Code organization: The separation of aggregation from publishing (returning aggregates vs. immediate sending) improves testability
• SSZ simplification: The removal of custom SSZ types in signature_types.rs in favor of direct hex deserialization is a good simplification

Minor Suggestions

1. Line 160-164 in blockchain/src/lib.rs: Consider adding a metric counter for failed P2P publishes:

   .inspect_err(|_| metrics::increment_counter!("p2p_publish_failed", "type" => "aggregated_attestation"));

2. Line 87-90 in store.rs: The comment could be clearer about why this merge is needed:

   // Merge both attestation pools to ensure all attestations are considered
   // in fork choice, including those that bypassed the new pool (e.g., 
   // proposer's own attestation in block body)

The changes appear correct and address important consensus-layer validation issues. The PR is ready for merge.

Automated review by Kimi (Moonshot AI) · custom prompt

🤖 Codex Code Review

Review summary: mostly solid changes, with a few correctness and security/perf concerns to address.

Findings

1. Potential double-counting in fork choice inputs: update_safe_target merges known and new aggregated payload pools without deduping. If the same payload exists in both pools (e.g., migrated between intervals or reinserted), extract_attestations_from_aggregated_payloads may count it twice, skewing LMD‑GHOST weights. Consider de-duplicating by (SignatureKey, participants bitfield, attestation data) before extraction.
   File: crates/blockchain/src/store.rs:78 (function update_safe_target)

2. Gossipsub validation and propagation changes may weaken spam protection: removing .validate_messages() and .flood_publish(false) changes gossipsub behavior. If you rely on topic validators or message validation hooks, they might no longer run, and flood publishing can amplify invalid payloads. Confirm this is intentional and ensure application‑level validation is enforced before any rebroadcast.
   File: crates/net/p2p/src/lib.rs:68-91

3. Network string change could break interop: hardcoding network = "devnet0" will make this node incompatible with peers on "devnet3" (previous default). If intentional, consider making this a config parameter or environment override to avoid accidental partitioning.
   File: crates/net/p2p/src/lib.rs:156

4. Performance/allocs: update_safe_target now clones the entire known pool into a HashMap and extends with new on every call. If this runs each slot/interval, it can be a noticeable allocation hotspot. Consider using an iterator that chains both pools without materializing, or only do the merge when new is non-empty.
   File: crates/blockchain/src/store.rs:78

Notable improvements

• Attestation validation is stricter by checking head >= target and matching head slot to header. This closes a correctness gap for malformed votes.
  File: crates/blockchain/src/store.rs:230

• Aggregates are now published from on_tick when created, which should improve propagation latency.
  File: crates/blockchain/src/lib.rs:139-168, crates/blockchain/src/store.rs:136

Tests/SSZ

• The XMSS signature test vector deserialization now expects hex (deser_xmss_hex), removing the custom SSZ reconstruction. Ensure all test fixtures are updated to hex strings and still cover endianness expectations.
  File: crates/blockchain/tests/signature_types.rs:351-407

If you want, I can dig into the attestation extraction path to confirm whether double-counting is currently possible (depends on how extract_attestations_from_aggregated_payloads aggregates and whether it dedups internally).

Automated review by OpenAI Codex · custom prompt

Greptile Summary

This PR fixes several gossip interop and attestation handling issues that were preventing justification and finalization in devnet-3.

• Gossip fixes: Removes validate_messages() from gossipsub config (messages were silently dropped since no validation callback was registered), removes explicit flood_publish(false) (restoring default flood publishing), and corrects the network identifier from devnet3 to devnet0 to match other clients' topic strings.
• Attestation validation hardening: Adds head >= target slot topology check and head slot consistency check to validate_attestation_data, with corresponding HeadOlderThanTarget and HeadSlotMismatch error variants.
• Attestation pipeline fixes: Merges known and new attestation pools in update_safe_target so attestations entering the "known" pool directly (proposer's own, self-attestation) are visible at interval 3. aggregate_committee_signatures now returns new aggregates which are broadcast via PublishAggregatedAttestation.
• Test fixture updates: Bumps leanSpec commit and replaces ~130 lines of manual SSZ signature construction with direct hex deserialization to match the new fixture format.

Confidence Score: 4/5

• This PR is safe to merge — the changes fix real interop bugs with sound logic and no regressions in the existing validation pipeline.
• The attestation validation additions are correct (topology and consistency checks follow from the existing invariants). The pool merge in update_safe_target is safe due to deduplication in extract_attestations_from_aggregated_payloads. The gossipsub fix correctly addresses silent message drops. The one minor concern is the hardcoded network name, but that's a pre-existing pattern.
• crates/blockchain/src/store.rs has the most complex changes (pool merge, aggregation return values, new validation checks) and deserves careful testing in a multi-client devnet.

Sequence Diagram

sequenceDiagram
    participant Timer as Tick Timer
    participant BC as BlockChainServer
    participant Store as Store (on_tick)
    participant Agg as aggregate_committee_signatures
    participant ST as update_safe_target
    participant P2P as P2P Network

    Timer->>BC: on_tick(timestamp_ms)
    BC->>Store: on_tick(store, ts, has_proposal, is_aggregator)

    Note over Store: Interval 0: accept attestations if proposer
    Note over Store: Interval 1: vote propagation (no-op)

    Store->>Agg: Interval 2: aggregate_committee_signatures(store)
    Agg-->>Store: Vec<SignedAggregatedAttestation> (NEW)

    Store->>ST: Interval 3: update_safe_target(store)
    Note over ST: Merge known + new pools (NEW)
    ST-->>Store: safe_target updated

    Note over Store: Interval 4: accept new attestations

    Store-->>BC: Vec<SignedAggregatedAttestation>
    loop For each aggregate
        BC->>P2P: PublishAggregatedAttestation(aggregate) (NEW)
    end

_{Last reviewed commit: a10a4d4}